Skip to content

Add Custom Shader Uniforms for Cursor Effects #6912

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

KroneCorylus
Copy link

Hi,
As requested, this is a pull request for discussion #6901. It adds cursor position data to the uniforms for custom shaders, enabling more dynamic and interactive effects.

Let me know if any adjustments are needed. Thanks!

blaze_trail.mp4

- Changed Uniforms struct in custom.zig to incorporate cursor data
- Added iCurrentCursor, iPreviousCursor, and iTimeCursorChange to the
  shadertoy_prefix.glsl
@KroneCorylus KroneCorylus requested a review from a team as a code owner March 26, 2025 00:25
@jcollie
Copy link
Member

jcollie commented Mar 26, 2025

Holy crap that is awesome. I was meh on cursor trails before but I def want this.

@hqnna hqnna added the renderer label Mar 26, 2025
Comment on lines +1955 to +1959
if (self.gl_state) |*gl_state| {
if (gl_state.custom) |*custom_state| {
custom_state.addCursor(self.size, screen.cursor, cursor_style, render.glyph);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at other parts of the code, I wonder if we should use the "deferred" pattern as used by SetFontSize, SetScreenSize, etc. The custom state seems to be more commonly modified there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll look into it and see how I can adapt it.

// Additionally, the y-position is adjusted to account for the inverted Y-axis
// in the window coordinate system used by gl_FragCoord, where the origin is at the bottom-left.
// The offset from the glyph is also added to position the cursor correctly.
const current_x: f32 = @as(f32, @floatFromInt(cursor.x * size.cell.width + size.padding.left)) + @as(f32, @floatFromInt(glyph.offset_x));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be misreading this, but since the two floats added together both came from integers, why not just:

Suggested change
const current_x: f32 = @as(f32, @floatFromInt(cursor.x * size.cell.width + size.padding.left)) + @as(f32, @floatFromInt(glyph.offset_x));
const current_x: f32 = @floatFromInt(cursor.x * size.cell.width + size.padding.left + glyph.offset_x);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't add glyph.offset directly because it's an i32, while all the other variables are u32.

Fix comment space

Co-authored-by: Kat <[email protected]>
@mitchellh
Copy link
Contributor

mitchellh commented Mar 28, 2025

I tried to run a couple of your shaders (blaze and smear) but they result in nothing being drawn. I've verified other [non-cursor] shaders work. I'm still trying to debug why that is but just wanted to let you know in case you knew. Thanks 😄 I just want something working here so I can better iterate on this PR.

EDIT: Ah, I see they're not really setup to be Ghostty shaders directly yet since they don't sample the screen. Looking at it.

@gj1118

This comment has been minimized.

@mitchellh
Copy link
Contributor

This requires some modifications so it works with Metal (something I plan to do) and some good example shaders (which were provided in the discussion). It'll be merged eventually...

@sommerper

This comment has been minimized.

@KroneCorylus

This comment has been minimized.

@sommerper

This comment has been minimized.

@pluiedev
Copy link
Member

It should be noted that PRs are not spaces for tech support. They are for reviewers to review code. Please leave your issues in the original discussion (#6901) going forwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants